Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update GScan incrementally, using tree/lookup #802

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

kinow
Copy link
Member

@kinow kinow commented Oct 11, 2021

This is a small change with no associated Issue.

The PR to propagate states in GScan has two changes. One where we propagate states, and another where we replace the computed property by a Tree+Lookup approach to avoid re-creating the computed property any time something changes in GScan, instead building the tree incrementally.

To simplify the other PR, I have moved the latter over to this PR, so we can compare before/after and merge it first, especially since I didn't get good performance with the propagate states part (which means it could take a bit longer for that change to be ready for review.)

Requirements check-list

  • I have read CONTRIBUTING.md and added my name as a Code Contributor.
  • Contains logically grouped changes (else tidy your branch by rebase).
  • Does not contain off-topic changes (use other PRs for other changes).
  • Appropriate tests are included (unit and/or functional).
  • Already covered by existing tests.
  • Appropriate change log entry included.
  • No documentation update required.

@kinow
Copy link
Member Author

kinow commented Oct 12, 2021

Yesterday: 18 failed tests
Today: 10 failed tests 😅

And this is only unit tests. Still pending the e2e tests 😵

@kinow
Copy link
Member Author

kinow commented Oct 18, 2021

Unit tests fixed locally. Now figure out how to fix these conflicts 😰

@kinow
Copy link
Member Author

kinow commented Oct 18, 2021

Rebased, fixed conflicts, but couldn't figure out how to update the tree/gscan Vuex modules 💫 😵

@codecov-commenter
Copy link

codecov-commenter commented Oct 18, 2021

Codecov Report

Merging #802 (2513af3) into master (ffb70c6) will decrease coverage by 0.70%.
The diff coverage is 85.18%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #802      +/-   ##
==========================================
- Coverage   91.00%   90.29%   -0.71%     
==========================================
  Files          87       90       +3     
  Lines        1700     1773      +73     
  Branches      105      106       +1     
==========================================
+ Hits         1547     1601      +54     
- Misses        123      141      +18     
- Partials       30       31       +1     
Flag Coverage Δ
e2e 75.69% <45.06%> (-3.37%) ⬇️
unittests 77.78% <70.80%> (-0.26%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/components/cylc/common/deltas.js 96.42% <ø> (ø)
src/components/cylc/tree/Tree.vue 76.47% <0.00%> (-2.32%) ⬇️
src/components/cylc/tree/TreeItem.vue 97.05% <ø> (ø)
src/components/cylc/tree/index.js 100.00% <ø> (ø)
src/components/cylc/tree/nodes.js 100.00% <ø> (ø)
src/mixins/graphql.js 100.00% <ø> (ø)
src/mixins/index.js 100.00% <ø> (ø)
src/model/WorkflowState.model.js 100.00% <ø> (ø)
src/store/options.js 100.00% <ø> (ø)
src/views/Tree.vue 100.00% <ø> (ø)
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ffb70c6...2513af3. Read the comment docs.

@kinow
Copy link
Member Author

kinow commented Oct 18, 2021

Finally unit tests passing! 🙏 (also had to fix git conflicts, lint, then docs, so not only unit tests 😅 )

@kinow
Copy link
Member Author

kinow commented Oct 19, 2021

Just one e2e test failing now, almost there...

@@ -15,32 +15,81 @@
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/

import { sortedIndexBy } from '@/components/cylc/common/sort'
import { sortWorkflowNamePartNodeOrWorkflowNode } from '@/components/cylc/gscan/sort'
// TODO: move to the `options` parameter that is passed to deltas; ideally it would be stored in DB or localstorage.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code is similar to what we have in the tree nodes.js

* You should have received a copy of the GNU General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/
import Vue from 'vue'
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Equivalent of the tree component's CylcTree, but with hierarchy for GScan.

@kinow kinow marked this pull request as ready for review October 19, 2021 01:52
@kinow
Copy link
Member Author

kinow commented Oct 19, 2021

One brittle test. When we visit URL's, and then click on buttons, it's taking too long to load the subscription (sometimes) and then the test is failing as it did for firefox ☝️ will investigate tomorrow.

@kinow
Copy link
Member Author

kinow commented Oct 19, 2021

The test was waiting for c-tree. But that's added too soon. I could chain methods (.then(() => { blabal.then(() => {...}}) }) but it gets a bit harder to read.

But there was an easier way to tell Cypress to wait for the tree to show. Just wait on c-task class instead. It only ever appears in the UI after the first Task component was added to the DOM 🎉 tests fixed, less brittle. Ready for review.

This might take a while to review, so @AaronDCole will probably inherit it. But at least it's not brittle, and doesn't have git conflicts. The best way to understand this PR is to understand how the Tree component & view work. This is basically the same as GScan on master, but instead of computed re-creating the GScan data structure/variable with every change, we build the Vuex gscan.gscan variable incrementally, applying deltas as received.

🖖

@kinow kinow mentioned this pull request Oct 19, 2021
6 tasks
@MetRonnie MetRonnie added this to the 1.x milestone Feb 7, 2022
@oliver-sanders oliver-sanders modified the milestones: 1.x, Pending Jun 8, 2022
@MetRonnie
Copy link
Member

@oliver-sanders I'm guessing this will be closed now?

@oliver-sanders
Copy link
Member

I think we'll have to work out how to pull the propagate states logic out onto the redesigned store. I think we could probably put that logic into a computed property on the GScan component?

@oliver-sanders
Copy link
Member

oliver-sanders commented Dec 20, 2022

Since the data store refactor we now have a universal tree containing both the GScan (workflow) and Tree (task) parts. This is updated incrementally using the global-callback.

So the workflow tree is being computed incrementally, but GScan is still using a computed property rather than iterating over this directly because of sorting and filtering.

In order to sort/filter incrementally it is still necessary to write a callback for GScan using the data from the lookup as done on this branch. The callbacks here are still valid, lookup has become $index but otherwise it's basically the same. Ideally we would avoid callbacks as these duplicate data processing and require loads of maintenance because they are bespoke to each view.

Because GScan is the only component (so far) that is worried about workflow iteration order, we could consider changing the sort order of the central store to what GScan expects, then use v-if to handle filtering. That would cut out the computed property completely.

Alternatively, we could consider maintaining a separate index in the central store just for GScan (e.g. $gscan).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants